-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Complete initial port to N-API #57
Conversation
src/microtime.cc
Outdated
timeval t; | ||
int r = gettimeofday(&t, NULL); | ||
|
||
if (r < 0) { | ||
return Nan::ThrowError(Nan::ErrnoException(errno, "gettimeofday")); | ||
throw Napi::Error::New(info.Env(), "gettimeofday"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old code here retained the errno
error code from C land in the error message, does the new Napi::Error does this automatically or is there a way we can preserve that behavior from nan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wadey,
you are right so I just updated the interested code. Now I create the error and than I set the code field like reported here:
Napi::Error e = Napi::Error::New(info.Env(), "gettimeofday");
e.Set("code", Napi::Number::New(info.Env(), errno));
throw e;
I've fixed up the errno exception a bit more, can I push the improved version directly to your branch so it's part of this PR? |
Hi @wadey, |
At least include the `strerror` result as the error message. This is not perfect, but copying all of Node::ErrnoException would be a lot of code. This is 90% there and we don't expect errors to happen anyways.
Ok, I have pushed my patch for a better ErrnoException. Let me read up on the deployment process for napi and I will merge this soon. Thanks! |
Thanks to you! |
Hi @wadey, |
@wadey have you had a chance to take a look at this N-API port? |
This was merged into my https://github.com/wadey/node-microtime/tree/napi branch, which will soon be merged into master. Thank you! |
For anyone following this ticket, please follow issue #56 instead. You can also test now with the |
Initial work to support the new N-API as reported here:
#56